Skip to content

Replace serverspec with inspec tests via. new CI matrix (inc. semantic-release) #262

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
May 6, 2019

Conversation

myii
Copy link
Contributor

@myii myii commented Apr 25, 2019

Currently, there is no CI-based testing on this formula. This PR brings in a number of the latest changes from the template-formula, in order to bring inspec testing via. a matrix of pre-salted images configured in Kitchen and run via. Travis. In it's current state, 9/10 of the platforms pass the tests. Here's a list of what remains before this can be merged:

  • Get remaining platform working (ubuntu-1804).
    • Didn't fix this but updated the matrix instead with the very latest available images to get 13/13 passing.
  • Update the README -- will do this once this PR is approved.
  • Recommends the merge of Use copy state instead of salt-call #256.
    • Ignored.
  • Recommends the merge of fix(uuid-ossp): use hyphen consistently #261.
    • Merged.
  • Trim off the last commit once the pre-salted images are updated to provide net-tools by default.
  • Add new centos-6 pre-salted image -- 14/14 working.
  • Test use_upstream_repo: True and False.
  • Test installing multiple PosgreSQL versions, including 9.6 and 10.
  • Test postgres.port: 5432 and 5433.
  • Add temporary workaround for inspec bug for opensuse-leap-15 (service-based tests).4
  • Implement semantic-release (includes documentation and library for TOFS).

Note that the formula itself needs to be improved to get it working with pillar.example instead of the provided test pillar (if we want to go that way). The improvements that need to be made can be seen by the commented out sections in the test pillar.


Update: Just a note about an issue with serverspec. While testing out these ideas initially, tried to hook up the existing serverspec tests but ended up hitting a weird bug where it was trying to run apt-get on opensuse -- tracked it down to this block in kitchen-verifier-serverspec.

Copy link
Member

@daks daks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Maybe squash some commits.
Are you planning to implement semantic-release too?

Very good job @myii! Thanks

@myii
Copy link
Contributor Author

myii commented Apr 25, 2019

@daks Thanks for the review. You're right about the commits -- I was planning to do that at the end. The idea right now is that each commit is a part that may have changes depending on the review, so the PR can be modified as required. For example, I definitely want to lose the last commit as well as maybe the one before that. It's easier to make the necessary modifications while this is a fluid WIP.

I left the semantic-release commented out because it's definitely needed, whether now or very soon. It opens up the discussion for it also.

Really appreciate you taking the time to review this PR.

Copy link
Contributor

@vutny vutny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job, @myii !

I'm not aware of the settings for this repo, but GitHub should dismiss any previous approvals if you'd force-push any commits afterwards. So better to squash them right before submitting a PR.
And any separate commit has no real value, since it may or may not represent the finished task and actually working changes. If it cannot be safely reverted in master later, I think a single squashed commit is the right way.

@daks
Copy link
Member

daks commented Apr 26, 2019

@vutny I think what it means is that during work (WIP PR) it's simpler to have one commit per change, it eases testing changes, reverting them if needed, etc. Then when work is finished, commits can be squashed.
I tend to use this pattern too.

@myii myii force-pushed the chore/add-new-ci-matrix branch 6 times, most recently from 4487e37 to 55f1cf0 Compare April 30, 2019 15:09
@myii myii changed the title WIP: Replace serverspec with inspec tests via. new CI matrix Replace serverspec with inspec tests via. new CI matrix Apr 30, 2019
@myii
Copy link
Contributor Author

myii commented Apr 30, 2019

With 13/13 images passing and the README updated, this PR is now ready for final review.

Copy link
Contributor

@aboe76 aboe76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@myii nice work on this formula !!!!

@myii
Copy link
Contributor Author

myii commented May 2, 2019

While 14/14 images being tested appears excessive, all sorts of combinations are there, besides different platforms and salt versions, as supplied by the pre-salted images:

  • Testing with and without installing from the upstream repo.
  • Testing multiple PostgreSQL versions.
  • Testing multiple ports.

So if there is a requirement to remove some of these images, care must be exercised so worthwhile testing isn't lost.

Copy link
Member

@daks daks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @myii

@myii myii force-pushed the chore/add-new-ci-matrix branch from 314cddb to ce1a00f Compare May 2, 2019 14:49
myii added 7 commits May 2, 2019 19:08
* Quoting from https://kitchen.ci/docs/getting-started/kitchen-yml/:
  - As of test-kitchen 1.21.0, we now prefer `kitchen.yml` over `.kitchen.yml`.
  - This preference applies to `kitchen.local.yml` as well.
  - This is backward compatible so the dot versions continue to work.
* `pillar.example` will be difficult to get working at this stage
* Ideally, work back towards `pillar.example` in the long run
* Fix the formula and then revert this eventually
@myii myii force-pushed the chore/add-new-ci-matrix branch from ce1a00f to 4cee3ca Compare May 2, 2019 18:12
@myii
Copy link
Contributor Author

myii commented May 2, 2019

While not directly related to serverspec => inspec, adding semantic-release to this PR was very straightforward after all of the other changes made. I've already tested it in my fork, the same way we have done with all of the other deployments. This PR is ready to merge, depending on any final comments.

@myii myii changed the title Replace serverspec with inspec tests via. new CI matrix Replace serverspec with inspec tests via. new CI matrix (inc. semantic-release) May 2, 2019
@myii myii force-pushed the chore/add-new-ci-matrix branch from 4cee3ca to 668f070 Compare May 3, 2019 08:46
@myii
Copy link
Contributor Author

myii commented May 3, 2019

Travis is hitting stuck builds on all three of the opensuse instances:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

Tried restarting each of them to no avail. There was nothing changed to have triggered this.

myii added 5 commits May 6, 2019 04:19
* Add new `centos-6` image
* Install `9.6` from upstream repo
* Fix `opensuse-leap-15` (enable `suse` and workaround `service` bug)
* Use port `5433` for `debian` and `opensuse-leap-15`
* Use upstream repo version `10` for `debian`
@myii myii force-pushed the chore/add-new-ci-matrix branch from d0c340a to 7d3aa19 Compare May 6, 2019 03:19
@myii
Copy link
Contributor Author

myii commented May 6, 2019

Thanks to @javierbertoli, the Travis issue has been resolved and we're back to 13/13 images passing. This is ready to merge again.

@daks daks merged commit 2a54616 into saltstack-formulas:master May 6, 2019
@saltstack-formulas-travis

🎉 This PR is included in version 0.37.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@myii myii deleted the chore/add-new-ci-matrix branch May 6, 2019 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants